Check __ANDROID_API__ instead of defining it automatically #633
Conversation
|
The current CI failure is not caused by this PR, however this PR may require changes in the CI configuration. |
build.rs
Outdated
| required_api_level).unwrap(); | ||
| if let Err(_) = cc::Build::new().warnings(false).file(tempfile.path()).try_expand() { | ||
| panic!("You need to define __ANDROID_API__ to at least {} for ring to compile", | ||
| required_api_level); |
There was a problem hiding this comment.
Rather than generate a temporary file to test this, autoconf style, it seems better to just put the minimum version check #if logic in crypto/internal.h. Is there a reason you didn't do it that way?
There was a problem hiding this comment.
I did it this way for the nicer-looking user-friendlier error message, and also to consider defining the constant ourselves automatically if it is not provided by the user.
But I can do it in crypto/internal.h if you want.
There was a problem hiding this comment.
I propose that we keep the code removal in your patch, but drop the code addition. Then, in the build documentation at https://github.com/briansmith/ring/blob/master/BUILDING.md, let's add a section on building for Android that indicates exactly what one must do to CFLAGS w.r.t. __ANDROID_API__ when building for Android.
In particular, IIUC, the Android SDK already does a check that __ANDROID_API__ is defined, so our check is somewhat superflous.
|
Coverage remained the same at ?% when pulling d194b81999deab3258e54522b4ae9794e81eb49b on tomaka:android-api-check into 1caad72 on briansmith:master. |
Cargo.toml
Outdated
| @@ -290,6 +290,7 @@ lazy_static = "1.0" | |||
| # control about what should be parallised in which way | |||
| cc = "1.0.9" | |||
| rayon = "1.0.0" | |||
| tempfile = "2.2.0" | |||
There was a problem hiding this comment.
If we're going to keep this, then this should be a conditional dependency only when the target OS is Android, if that is possible.
build.rs
Outdated
| required_api_level).unwrap(); | ||
| if let Err(_) = cc::Build::new().warnings(false).file(tempfile.path()).try_expand() { | ||
| panic!("You need to define __ANDROID_API__ to at least {} for ring to compile", | ||
| required_api_level); |
There was a problem hiding this comment.
I propose that we keep the code removal in your patch, but drop the code addition. Then, in the build documentation at https://github.com/briansmith/ring/blob/master/BUILDING.md, let's add a section on building for Android that indicates exactly what one must do to CFLAGS w.r.t. __ANDROID_API__ when building for Android.
In particular, IIUC, the Android SDK already does a check that __ANDROID_API__ is defined, so our check is somewhat superflous.
tomaka
force-pushed
the
android-api-check
branch
from
d194b81
to
9a0a708
Compare
|
Updated! |
| @@ -58,6 +58,9 @@ Note in particular that if you are cross-compiling an x86 build on a 64-bit | |||
| version of Linux, then you need to have the proper gcc-multilibs and | |||
| g++-multilibs packages or equivalent installed. | |||
|
|
|||
| On Android, you will need to define the `__ANDROID_API__` constant to at least | |||
| `21` on 64-bit and `18` on 32-bit. | |||
There was a problem hiding this comment.
How does this sound?:
If you generate a standalone NDK toolchain in order to compile your project, the wrapper automatically passes the ANDROID_API constant to the actual compiler. Otherwise, the __ANDROID_API__ must be defined with a value of at least 21 on 64-bit targets or 18 on 32-bit targets (e.g. export CFLAGS=-D__ANDROID_API__=21).
|
Thanks! Merged! |
|
I pushed df1ee11 to clarify the wording a bit like I suggested above. |
|
Reverted in b98f61f and b2fd4fa. The Android build breaks in Travis CI; see https://travis-ci.org/briansmith/ring/jobs/379021311 for an example. I think the issue is that our Travis CI configuration is using GCC instead of clang; see https://github.com/android-ndk/ndk/wiki/Changelog-r14-beta1#wont-fix where it says standalone toolchains using GCC need to set define this macro manually. Probably we should switch to clang on Travis CI for all Android targets. |
This should be done through the environment variables of cc-rs in my opinion. |
|
Unfortunately I really haven't had time to deal with this. When I tried using clang the tests would fail. @tomaka can you build and test ring using clang for android? |
|
I think ring should just drop support for GCC in favor of requiring Clang for targeting Android. In I originally accepted this PR because I was assured that the
If somebody writes a PR that gets us up to the point of failing tests (NOTE: As of now, all testing on Android is disabled on Travis CI) then I will investigate the test failures myself. Then we can reland this. |
|
https://travis-ci.org/briansmith/ring/builds/382297181 is branch that attempts to reland this. |
|
Cool, thank you! |
This pull requests removes the code that automatically defines the
__ANDROID_API__constant, and replaces it by a code that checks whether the constant has been defined by something else.In other words, we now assume that the user manually defines
__ANDROID_API__, for example through theCFLAGSenvironment variable.The main reason is practical. If you generate a standalone NDK toolchain in order to compile your project, the wrapper automatically passes the
__ANDROID_API__constant to the actual compiler. Therefore if you try to compile ring for the standalone toolchain, you will most likely get a compilation error because the constant passed by the standalone conflicts with the one defined by ring.I don't know how widely used ring for Android is, but I'm open to feedback about this change. I don't really have a very strong opinion ; my goal is mainly to make Android compilation as smooth as possible.